-
Notifications
You must be signed in to change notification settings - Fork 71.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wip/bewest/express slow down #7469
Conversation
Replace usage of our delayIp with a popular middleware.
This restores a delay when verifyauth indicates that a request is not authorized.
This allows implementing a longer delay with the request is unauthenticated.
Is the change based on speculation or observed issues with the existing implementation? |
Good questions, @sulkaharo. Observed, although the environment and harness may have had unrelated issues. I believe I had issues with valid concurrent requests getting delayed. I'm fine with delaying this while I collect more data. My usage was via staging site exercising https://github.com/t1pal/nightscout-roles-gateway. I can use these patches to help evaluate the behavior I saw. |
There should effectively be no cases where valid usage of Nightscout gets delayed. The only case I know where this has failed is with misconfigured proxies that fail to forward the IP in the header, resulting in all requests seemingly coming from the same IP. |
I can remove the added delays to restrict only the unauthorized delays. I did observe delays on valid requests unrelated to a misconfigured load balancer. My reading suggested that concurrent requests would be delayed before checking validity, although there are other possibilities I'm still closing out. |
Delays on valid requests sounds curious, since the logic only delays an IP in case the IP repeatedly trying to authenticate using the wrong credentials, so the only means to trigger this behavior is through a user having misconfigured an access key and the client repeatedly trying to authenticate without user realising what's going on, or a hacker trying to guess a passkey. Also btw note the admin message UI that informs a valid user their client is trying to use wrong credentials or someone is trying to hack their site relies on the IP list, so if the old logic is removed, you still need to have logic in place to support the admin message feature. |
Related: we should probably change the flag that disables the admin message feature into only disabling the alarm on Nightscout being visible to the world. Right now the flag is being used by people who have no idea about the implications of disabling the feature and thus not being informed if they have a misconfigured client spamming their site (causing support load for the community, as the admin message feature also intended to help debug misconfigured clients) or if someone is trying to hack their site (at which point it makes sense to change the URL and keep it secret). |
Great points, @sulkaharo. Could you say more about what you mean for the notifies flag? I think I agree if you mean that flags should be able to filter kinds of messages rather than turning the messaging feature itself off. Are there categories of messages that can/should be filtered? After some further experiments, it looks like the delay was due to an issue with my environment, and unrelated to the delay implementation. I think this alternative was a nice case study/exercise but probably doesn't offer any value for the remaining effort left. FWIW, it looks like I had two Nightscout instances that were load balanced under the same interface, but out of sync in terms of tokens. Eg: requests for xyz.backend-ns.local were load balanced between NS A and NS B, both instances pointed at the same mongo database with same API Secret. If we get a valid token from NS A, what happens when NS B attempts to verify it? It looks like NS B may not verify that token? In any case, this isn't caused by any issue with the delay logic, as you pointed out. We can close or archive this for now. |
Introduce using https://www.npmjs.com/package/express-slow-down. The express-slow-down library allows specifying when the delay will start taking affect without any additional maintenance on on our end.
Some potential discussion points:
Should all API endpoint have rate limiting?